Status events #70
Status events #70
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update refactors the handling of server runtimes by introducing start and end times, replacing the old runtime logic. It modifies various components and functions to manage and display start and end times dynamically. Changes span multiple files, including TypeScript and Python, involving updates to types, state handling, and response formats. Changes
Poem
Happy coding! 🌟🐇 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- server/app/query/servers.tsx (5 hunks)
- server/app/query/view/[id]/components.tsx (2 hunks)
- server/app/query/view/[id]/page.tsx (5 hunks)
- sidecar/app/query/base.py (3 hunks)
- sidecar/app/routes/websockets.py (2 hunks)
Additional comments not posted (21)
sidecar/app/routes/websockets.py (2)
37-39: Addition ofstart_timein NOT_FOUND response is correct.The inclusion of
start_timein the JSON response forStatus.NOT_FOUNDaligns with the objective of tracking start and end times.
43-47: Use ofquery.status_event_jsonfor status updates is correct.The change to use
query.status_event_jsonfor sending status updates aligns with the objective of tracking status history and ensures consistency.server/app/query/view/[id]/components.tsx (4)
1-1: Addition of React imports is correct.The added React imports
useEffect,useState, anduseRefare necessary for the updatedRunTimePillcomponent.
69-74: Update toRunTimePillprops is correct.The
RunTimePillcomponent now correctly acceptsstartTimeandendTimeas props, aligning with the objective of tracking start and end times.
80-98:useEffectlogic inRunTimePillcomponent is correct.The
useEffectlogic correctly handles changes tostartTimeandendTime, updating therunTimestate as intended.
99-99: Return statement inRunTimePillcomponent is correct.The return statement correctly displays the
runTimeStrbased on the updated state.sidecar/app/query/base.py (6)
26-27: Addition ofStatusChangeEventnamed tuple is correct.The
StatusChangeEventnamed tuple is correctly defined to store status and timestamp, aligning with the objective of tracking status history.
41-43: Addition of_status_historyfield inQueryclass is correct.The
_status_historyfield is correctly defined to store a list ofStatusChangeEvent, aligning with the objective of tracking status history.
91-105:load_history_from_filemethod inQueryclass is correct.The
load_history_from_filemethod correctly loads status history from a file and populates_status_historyas intended.
106-111:_last_status_eventproperty inQueryclass is correct.The
_last_status_eventproperty correctly returns the last status event from_status_history.
112-121:status_event_jsonproperty inQueryclass is correct.The
status_event_jsonproperty correctly generates a JSON representation of the last status event, including start and end times if applicable.
129-134:statussetter inQueryclass is correct.The
statussetter correctly updates_status_historyand writes changes to a file, ensuring consistency with the objective of tracking status history.server/app/query/servers.tsx (5)
62-68: Addition ofStartTimeByRemoteServerandEndTimeByRemoteServertypes is correct.The
StartTimeByRemoteServerandEndTimeByRemoteServertypes are correctly defined to store start and end times for remote servers.
70-75: Addition ofinitialStartTimeByRemoteServerandinitialEndTimeByRemoteServeris correct.The
initialStartTimeByRemoteServerandinitialEndTimeByRemoteServerare correctly defined to initialize start and end times for remote servers.
Line range hint
191-219: Update toopenStatusSocketmethod inRemoteServerclass is correct.The
openStatusSocketmethod correctly handles start and end times, aligning with the objective of tracking start and end times.
203-220:updateStartTimeandupdateEndTimemethods inRemoteServerclass are correct.The
updateStartTimeandupdateEndTimemethods correctly update the state with start and end times.
222-225:onmessageevent handler inopenStatusSocketmethod is correct.The
onmessageevent handler correctly updates start and end times based on the event data.server/app/query/view/[id]/page.tsx (4)
19-24: LGTM! Import statements are updated correctly.The new imports for
StartTimeByRemoteServerandEndTimeByRemoteServerare necessary for handling the start and end times separately.
53-56: LGTM! State variables are updated correctly.The new state variables
startTimeByRemoteServerandendTimeByRemoteServerare essential for tracking the start and end times of remote servers.
115-116: LGTM! WebSocket handling is updated correctly.Including
setStartTimeByRemoteServerandsetEndTimeByRemoteServerensures that the start and end times are set correctly for each remote server.
Line range hint
220-242:
LGTM! RunTimePill component usage is updated correctly.The
RunTimePillcomponent now usesstartTimeandendTimeprops, allowing it to display the runtime correctly.
| setRunTime(endTime - startTime); | ||
| } else { | ||
| let newIntervalId = setInterval(() => { | ||
| setRunTime(Date.now() / 1000 - startTime); |
There was a problem hiding this comment.
can't you end up here with startTime == null?
There was a problem hiding this comment.
should be covered above on line 86
There was a problem hiding this comment.
ah, is there a reason for checking startTime again on the line 89?
There was a problem hiding this comment.
oh, nope! just a vestige of a refactor.
| return None | ||
| return query | ||
|
|
||
| def load_history_from_file(self): |
There was a problem hiding this comment.
I would suggest to add a comment showing an example how log file looks like, so we can compare and check that it is doing the right thing
There was a problem hiding this comment.
there's also a test that does this in the next PR
| ) | ||
|
|
||
| @property | ||
| def _last_status_event(self): |
There was a problem hiding this comment.
is it expected to be public with _ or @Property does not work as I think it works?
There was a problem hiding this comment.
this is intended to be private. @property is just a helper that makes instance.method a shorthand for instance.method().
| with self.status_file_path.open("w") as f: | ||
| self.logger.debug(f"setting status: {status=}") | ||
| f.write(str(status.name)) | ||
| if self.status <= Status.COMPLETE: |
There was a problem hiding this comment.
don't see the enum definition, but does this check include failed queries as well?
There was a problem hiding this comment.
It's defined here as
class Status(IntEnum):
UNKNOWN = auto()
STARTING = auto()
COMPILING = auto()
WAITING_TO_START = auto()
IN_PROGRESS = auto()
COMPLETE = auto()
KILLED = auto()
NOT_FOUND = auto()
CRASHED = auto()so this prevents changing the status once it's COMPLETE or higher (including KILLED and CRASHED).
| if self.status <= Status.COMPLETE: | ||
| now = time.time() | ||
| self._status_history.append(StatusChangeEvent(status=status, timestamp=now)) | ||
| with self.status_file_path.open("a") as f: |
There was a problem hiding this comment.
It may be better to have a class that manages the history and flushes it to the backing storage, so Query can take itself off this unrelated to its core functionality business
There was a problem hiding this comment.
yep, that's exactly what #71 does 🙂
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- server/app/query/view/[id]/components.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
- server/app/query/view/[id]/components.tsx
This updates the sidecar to store the entire status history of a query, instead of just the current status. It also includes a timestamp. With that timestamp, we're then able to simply update the run time based on when the current status started, and in the case where it's complete/finished/crashed, it shows the difference between start and end time.
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Backend Improvements